performance(ci): Made cairo tests run in release. #9583
Conversation
|
This change is |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d421683cc1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cairotest: | ||
| if: ${{ always() }} | ||
| needs: parallel-cairo-tests | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
The cairotest job still declares needs: parallel-cairo-tests, but the parallel-cairo-tests job was deleted in this commit. GitHub Actions treats a workflow referencing a missing job as invalid, so the entire CI workflow will fail to load rather than run tests. This breaks CI for every push until the dependency is removed or the job is restored.
Useful? React with 👍 / 👎.
| steps: | ||
| - if: needs.parallel-cairo-tests.result == 'success' | ||
| run: exit 0 | ||
| - if: needs.parallel-cairo-tests.result != 'success' | ||
| run: exit 1 | ||
| - run: | | ||
| cargo run --profile=release --bin cairo-test -- corelib/ |
There was a problem hiding this comment.
The cairotest job now runs cargo run commands but no longer checks out the repository or installs a Rust toolchain. On GitHub Actions, jobs start in an empty workspace, so these steps will fail with missing source/toolchain errors. This will make the job fail even if the removed dependency is fixed.
Useful? React with 👍 / 👎.
| cairotest: | ||
| if: ${{ always() }} | ||
| needs: parallel-cairo-tests | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
The new nightly cairotest job also declares needs: parallel-cairo-tests, but there is no such job in nightly.yml. GitHub Actions will reject the workflow definition, preventing the nightly schedule from running at all until the dependency is removed or the job is added.
Useful? React with 👍 / 👎.
| steps: | ||
| - run: | | ||
| cargo run --profile=ci-dev --bin cairo-test -- corelib/ |
There was a problem hiding this comment.
The nightly cairotest job runs cargo run without checking out the repo or installing a toolchain. As with the CI workflow, this will fail on GitHub Actions because the workspace is empty by default and cargo may not be available. Add actions/checkout and a toolchain step before these commands.
Useful? React with 👍 / 👎.
orizi
force-pushed
the
orizi/02-01-performance_ci_made_cairo_tests_run_in_release
branch
from
d421683 to
4afda97
Compare
orizi
left a comment
orizi
left a comment
There was a problem hiding this comment.
@orizi resolved 4 discussions.
Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @eytan-starkware).
eytan-starkware
left a comment
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 2 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @orizi).
.github/workflows/nightly.yml line 34 at r2 (raw file):
cairotest: if: ${{ always() }}
ditto
.github/workflows/nightly.yml line 35 at r2 (raw file):
cairotest: if: ${{ always() }} needs: parallel-cairo-tests
Isnt parallel cairo tests the thing you just replaced?
.github/workflows/ci.yml line 128 at r2 (raw file):
# Checks that all Cairo code tests run correctly. cairotest: if: ${{ always() }}
is if always necessary?
orizi
force-pushed
the
orizi/02-01-performance_ci_made_cairo_tests_run_in_release
branch
from
4afda97 to
fb49b6a
Compare
orizi
force-pushed
the
orizi/02-01-performance_ci_made_cairo_tests_run_in_release
branch
from
fb49b6a to
adfbff3
Compare
orizi
left a comment
orizi
left a comment
There was a problem hiding this comment.
@orizi made 3 comments.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @eytan-starkware).
.github/workflows/ci.yml line 128 at r2 (raw file):
Previously, eytan-starkware wrote…
is if always necessary?
Done.
.github/workflows/nightly.yml line 34 at r2 (raw file):
Previously, eytan-starkware wrote…
ditto
Done.
.github/workflows/nightly.yml line 35 at r2 (raw file):
Previously, eytan-starkware wrote…
Isnt parallel cairo tests the thing you just replaced?
Done.
Summary
Changed CI workflow by removing the parallel-cairo-tests job and integrating its functionality directly into the cairotest job. The tests that were previously run in parallel are now executed sequentially in the cairotest job, but being run in release. Also added a cairotest job to the nightly workflow that runs the same Cairo tests with the ci-dev profile.
Type of change
Please check one:
Why is this change needed?
The current CI ran in ci-dev, which is much slower. This change enhances the CI process to run in release. Additionally, adding the cairotest job to the nightly workflow ensures these tests are run regularly with the ci-dev profile.
What was the behavior or documentation before?
Cairo tests were run in parallel in a separate job (parallel-cairo-tests) with a matrix strategy, and the cairotest job only checked if the parallel job succeeded. The nightly workflow did not include Cairo tests.
What is the behavior or documentation after?
The cairotest job now directly runs all the Cairo tests sequentially without depending on a separate parallel job. The same tests are also run in the nightly workflow with the ci-dev profile.
Additional context
This change simplifies the CI workflow structure while maintaining the same test coverage. Running the tests sequentially might take slightly longer but eliminates the overhead of managing parallel jobs.